Sidebar adjustments - #35
Conversation
… include Investigator role
…urity roles for ML service endpoints
…d and embedding generation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| try { | ||
| c.setDateOfBirth(LocalDate.parse(dateOfBirth)); | ||
| } catch (Exception e) { | ||
| log.warn("Invalid date_of_birth format: {}", dateOfBirth); |
Check failure
Code scanning / CodeQL
Log Injection High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 5 months ago
In general, user-provided values should be sanitized or validated before being written to logs. For plain-text logs, at minimum this means stripping or replacing newline and other control characters that could break log structure or inject extra entries, or validating that the string matches an expected safe pattern and logging a sanitized placeholder if it does not.
For this specific case, the best fix with minimal behavior change is to sanitize dateOfBirth before logging it. The project already has com.crimeLink.analyzer.util.LogSanitizer used in CriminalController for name and nic. We should apply the same sanitization here by wrapping dateOfBirth with LogSanitizer.sanitize(...) at the log call site. This keeps the functional behavior (logging the problematic value) while ensuring any dangerous characters are neutralized in the logs.
Concretely:
- In
src/main/java/com/crimeLink/analyzer/service/CriminalService.java, importLogSanitizer. - Change the warning log in the
catchblock aroundLocalDate.parse(dateOfBirth)to logLogSanitizer.sanitize(dateOfBirth)instead of the rawdateOfBirth.
No other files or logic need to be modified.
| @@ -2,6 +2,7 @@ | ||
|
|
||
| import com.crimeLink.analyzer.entity.Criminal; | ||
| import com.crimeLink.analyzer.repository.CriminalRepository; | ||
| import com.crimeLink.analyzer.util.LogSanitizer; | ||
| import lombok.RequiredArgsConstructor; | ||
| import lombok.extern.slf4j.Slf4j; | ||
| import org.springframework.stereotype.Service; | ||
| @@ -51,7 +52,7 @@ | ||
| try { | ||
| c.setDateOfBirth(LocalDate.parse(dateOfBirth)); | ||
| } catch (Exception e) { | ||
| log.warn("Invalid date_of_birth format: {}", dateOfBirth); | ||
| log.warn("Invalid date_of_birth format: {}", LogSanitizer.sanitize(dateOfBirth)); | ||
| } | ||
| } | ||
| if (gender != null) c.setGender(gender); |
| } | ||
|
|
||
| Criminal saved = criminalRepository.save(c); | ||
| log.info("Criminal created: {} (name: {})", criminalId, name); |
Check failure
Code scanning / CodeQL
Log Injection High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 5 months ago
In general, to fix log injection, ensure that any user-provided values included in log messages are sanitized to remove or neutralize characters that can break log structure (like \r, \n, and other control characters) or otherwise confuse log readers. In this codebase, a dedicated LogSanitizer utility already exists and is used in the controller when logging name and nic, so the best fix is to consistently reuse that utility wherever user input is logged.
Concretely, in CriminalService.createCriminal, line 73 logs the name parameter directly. We should import LogSanitizer in this service (it’s currently only imported in the controller) and apply it to name in the log statement. This preserves the existing business logic and persistence behavior while only changing how data is rendered in logs. The change is limited to:
- Adding
import com.crimeLink.analyzer.util.LogSanitizer;at the top ofCriminalService.java. - Updating the logging call on line 73 to use
LogSanitizer.sanitize(name)instead ofname.
No new methods or external dependencies are needed; we simply reuse the existing sanitizer utility.
| @@ -2,6 +2,7 @@ | ||
|
|
||
| import com.crimeLink.analyzer.entity.Criminal; | ||
| import com.crimeLink.analyzer.repository.CriminalRepository; | ||
| import com.crimeLink.analyzer.util.LogSanitizer; | ||
| import lombok.RequiredArgsConstructor; | ||
| import lombok.extern.slf4j.Slf4j; | ||
| import org.springframework.stereotype.Service; | ||
| @@ -70,7 +71,7 @@ | ||
| } | ||
|
|
||
| Criminal saved = criminalRepository.save(c); | ||
| log.info("Criminal created: {} (name: {})", criminalId, name); | ||
| log.info("Criminal created: {} (name: {})", criminalId, LogSanitizer.sanitize(name)); | ||
|
|
||
| // Generate face embedding via Python ML service (non-blocking) | ||
| boolean hasEmbedding = false; |
| try { | ||
| c.setDateOfBirth(LocalDate.parse(dateOfBirth)); | ||
| } catch (Exception e) { | ||
| log.warn("Invalid date_of_birth format: {}", dateOfBirth); |
Check failure
Code scanning / CodeQL
Log Injection High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 5 months ago
In general, to fix log injection issues, any user-provided value must be sanitized or validated before being written to logs. Typical mitigations are: stripping/normalizing newline and other control characters, whitelisting characters via a regex, or using a centralized sanitizer utility that performs this normalization and clearly marks user input.
For this specific case, the best fix with minimal functional change is to sanitize dateOfBirth before it is included in the log message. The project already has a LogSanitizer utility used in CriminalController for criminalId, so we should reuse that rather than creating ad‑hoc sanitization logic. Concretely, in CriminalService.updateCriminal, we will change the log.warn call inside the catch block to pass dateOfBirth through LogSanitizer.sanitize(...). To do this safely and consistently, we need to import com.crimeLink.analyzer.util.LogSanitizer at the top of CriminalService.java and adjust only the affected logging call:
- Add an import for
LogSanitizerinCriminalService.java. - Replace
log.warn("Invalid date_of_birth format: {}", dateOfBirth);withlog.warn("Invalid date_of_birth format: {}", LogSanitizer.sanitize(dateOfBirth));.
No method signatures or behavior of the application change other than the fact that logged dateOfBirth values are now sanitized. Persistence logic and parsing behavior remain identical.
| @@ -2,6 +2,7 @@ | ||
|
|
||
| import com.crimeLink.analyzer.entity.Criminal; | ||
| import com.crimeLink.analyzer.repository.CriminalRepository; | ||
| import com.crimeLink.analyzer.util.LogSanitizer; | ||
| import lombok.RequiredArgsConstructor; | ||
| import lombok.extern.slf4j.Slf4j; | ||
| import org.springframework.stereotype.Service; | ||
| @@ -147,7 +148,7 @@ | ||
| try { | ||
| c.setDateOfBirth(LocalDate.parse(dateOfBirth)); | ||
| } catch (Exception e) { | ||
| log.warn("Invalid date_of_birth format: {}", dateOfBirth); | ||
| log.warn("Invalid date_of_birth format: {}", LogSanitizer.sanitize(dateOfBirth)); | ||
| } | ||
| } | ||
| if (gender != null) c.setGender(gender); |
| try { | ||
| String newPhotoUrl = supabaseStorageService.uploadPhoto(criminalId, photo); | ||
| c.setPrimaryPhotoUrl(newPhotoUrl); | ||
| log.info("Photo updated for criminal {}", criminalId); |
Check failure
Code scanning / CodeQL
Log Injection High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 5 months ago
In general, user-controlled data should be sanitized before being written to logs, especially when it can contain newline or control characters. This project already has com.crimeLink.analyzer.util.LogSanitizer used in CriminalController, so the best and least intrusive fix is to reuse that utility in CriminalService wherever criminalId (or any other user input) is logged. Core functionality (database lookups, storage operations) can keep using the raw criminalId; only the values passed to log.* calls need to be sanitized.
Concretely, in CriminalService.updateCriminal, we should import LogSanitizer at the top of the file, and wrap criminalId with LogSanitizer.sanitize(...) in the three log statements that currently interpolate the raw ID:
log.info("Photo updated for criminal {}", criminalId);log.info("Embedding regenerated for criminal {}", criminalId);log.info("Criminal updated: {}", criminalId);
No behavior changes occur except for how the ID appears in logs; application logic remains identical. Only src/main/java/com/crimeLink/analyzer/service/CriminalService.java needs edits; CriminalController.java already uses LogSanitizer correctly.
| @@ -2,6 +2,7 @@ | ||
|
|
||
| import com.crimeLink.analyzer.entity.Criminal; | ||
| import com.crimeLink.analyzer.repository.CriminalRepository; | ||
| import com.crimeLink.analyzer.util.LogSanitizer; | ||
| import lombok.RequiredArgsConstructor; | ||
| import lombok.extern.slf4j.Slf4j; | ||
| import org.springframework.stereotype.Service; | ||
| @@ -159,22 +160,22 @@ | ||
| try { | ||
| String newPhotoUrl = supabaseStorageService.uploadPhoto(criminalId, photo); | ||
| c.setPrimaryPhotoUrl(newPhotoUrl); | ||
| log.info("Photo updated for criminal {}", criminalId); | ||
| log.info("Photo updated for criminal {}", LogSanitizer.sanitize(criminalId)); | ||
| } catch (Exception e) { | ||
| log.error("Photo upload failed for criminal {}: {}", criminalId, e.getMessage()); | ||
| log.error("Photo upload failed for criminal {}: {}", LogSanitizer.sanitize(criminalId), e.getMessage()); | ||
| } | ||
|
|
||
| // Regenerate face embedding with new photo | ||
| try { | ||
| facialRecognitionService.generateEmbedding(criminalId, photo); | ||
| log.info("Embedding regenerated for criminal {}", criminalId); | ||
| log.info("Embedding regenerated for criminal {}", LogSanitizer.sanitize(criminalId)); | ||
| } catch (Exception e) { | ||
| log.warn("Embedding regeneration failed for criminal {} (non-fatal): {}", criminalId, e.getMessage()); | ||
| log.warn("Embedding regeneration failed for criminal {} (non-fatal): {}", LogSanitizer.sanitize(criminalId), e.getMessage()); | ||
| } | ||
| } | ||
|
|
||
| Criminal saved = criminalRepository.save(c); | ||
| log.info("Criminal updated: {}", criminalId); | ||
| log.info("Criminal updated: {}", LogSanitizer.sanitize(criminalId)); | ||
|
|
||
| return Optional.of(toDetailMap(saved)); | ||
| } |
| c.setPrimaryPhotoUrl(newPhotoUrl); | ||
| log.info("Photo updated for criminal {}", criminalId); | ||
| } catch (Exception e) { | ||
| log.error("Photo upload failed for criminal {}: {}", criminalId, e.getMessage()); |
Check failure
Code scanning / CodeQL
Log Injection High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 5 months ago
In general, to fix log injection issues, ensure that any user-provided string passed into log messages is sanitized or validated before logging. In this codebase, there is already a LogSanitizer utility in com.crimeLink.analyzer.util used in CriminalController to sanitize criminalId for logging, so the most consistent and low-impact fix is to apply the same sanitizer in CriminalService wherever criminalId (or other tainted input) is logged.
Concretely for this issue, in CriminalService.updateCriminal we should avoid logging the raw criminalId. We can import com.crimeLink.analyzer.util.LogSanitizer and then replace each occurrence of criminalId in log statements with LogSanitizer.sanitize(criminalId). This should be done for:
log.error("Photo upload failed for criminal {}: {}", criminalId, e.getMessage());(the one flagged),- as well as the related log lines in this method:
log.info("Photo updated for criminal {}", criminalId);,log.info("Embedding regenerated for criminal {}", criminalId);,log.warn("Embedding regeneration failed for criminal {} (non-fatal): {}", criminalId, e.getMessage());, andlog.info("Criminal updated: {}", criminalId);to keep behavior consistent and safe.
To implement this, we need to:
- Add an import for
com.crimeLink.analyzer.util.LogSanitizer;at the top ofCriminalService.java(without altering existing imports). - Update the five logging calls in
updateCriminalto callLogSanitizer.sanitize(criminalId)instead of passingcriminalIddirectly. No functional behavior beyond log content changes; all database and service logic remains the same.
| @@ -2,6 +2,7 @@ | ||
|
|
||
| import com.crimeLink.analyzer.entity.Criminal; | ||
| import com.crimeLink.analyzer.repository.CriminalRepository; | ||
| import com.crimeLink.analyzer.util.LogSanitizer; | ||
| import lombok.RequiredArgsConstructor; | ||
| import lombok.extern.slf4j.Slf4j; | ||
| import org.springframework.stereotype.Service; | ||
| @@ -159,22 +160,22 @@ | ||
| try { | ||
| String newPhotoUrl = supabaseStorageService.uploadPhoto(criminalId, photo); | ||
| c.setPrimaryPhotoUrl(newPhotoUrl); | ||
| log.info("Photo updated for criminal {}", criminalId); | ||
| log.info("Photo updated for criminal {}", LogSanitizer.sanitize(criminalId)); | ||
| } catch (Exception e) { | ||
| log.error("Photo upload failed for criminal {}: {}", criminalId, e.getMessage()); | ||
| log.error("Photo upload failed for criminal {}: {}", LogSanitizer.sanitize(criminalId), e.getMessage()); | ||
| } | ||
|
|
||
| // Regenerate face embedding with new photo | ||
| try { | ||
| facialRecognitionService.generateEmbedding(criminalId, photo); | ||
| log.info("Embedding regenerated for criminal {}", criminalId); | ||
| log.info("Embedding regenerated for criminal {}", LogSanitizer.sanitize(criminalId)); | ||
| } catch (Exception e) { | ||
| log.warn("Embedding regeneration failed for criminal {} (non-fatal): {}", criminalId, e.getMessage()); | ||
| log.warn("Embedding regeneration failed for criminal {} (non-fatal): {}", LogSanitizer.sanitize(criminalId), e.getMessage()); | ||
| } | ||
| } | ||
|
|
||
| Criminal saved = criminalRepository.save(c); | ||
| log.info("Criminal updated: {}", criminalId); | ||
| log.info("Criminal updated: {}", LogSanitizer.sanitize(criminalId)); | ||
|
|
||
| return Optional.of(toDetailMap(saved)); | ||
| } |
| listUrl, HttpMethod.POST, listRequest, String.class); | ||
|
|
||
| if (!listResponse.getStatusCode().is2xxSuccessful() || listResponse.getBody() == null) { | ||
| log.warn("Failed to list storage files for criminal {}", criminalId); |
Check failure
Code scanning / CodeQL
Log Injection High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 5 months ago
To generally fix log injection issues, any user-controlled data should be sanitized or validated before being written to logs. The simplest and least invasive approach here is to reuse the existing LogSanitizer utility already used in CriminalController, rather than reinventing sanitization logic. This keeps behavior consistent across the codebase and avoids changing functional logic—only the log messages are modified to use sanitized variants of criminalId.
Concretely, in SupabaseStorageService.deleteFolder, we should:
- Import
com.crimeLink.analyzer.util.LogSanitizer. - Compute a sanitized version of
criminalId, e.g.String safeCriminalId = LogSanitizer.sanitize(criminalId);at the start of the method. - Use
safeCriminalIdin all logging calls that currently interpolate the rawcriminalId(lines 107, 115, 147, 149, 153). - Keep using the raw
criminalIdfor functional behavior (building prefixes, request bodies, etc.) so existing logic is unchanged.
All changes are confined to src/main/java/com/crimeLink/analyzer/service/SupabaseStorageService.java in the deleteFolder method and the imports section.
| @@ -13,6 +13,8 @@ | ||
| import java.util.ArrayList; | ||
| import java.util.List; | ||
|
|
||
| import com.crimeLink.analyzer.util.LogSanitizer; | ||
|
|
||
| /** | ||
| * Service for uploading files to Supabase Storage. | ||
| * Uploads criminal photos to the configured bucket and returns public URLs. | ||
| @@ -88,6 +90,7 @@ | ||
| * @param criminalId The criminal's ID (used as the folder prefix) | ||
| */ | ||
| public void deleteFolder(String criminalId) { | ||
| String safeCriminalId = LogSanitizer.sanitize(criminalId); | ||
| try { | ||
| // 1) List objects in the folder | ||
| String listUrl = supabaseUrl + "/storage/v1/object/list/" + bucket; | ||
| @@ -104,7 +107,7 @@ | ||
| listUrl, HttpMethod.POST, listRequest, String.class); | ||
|
|
||
| if (!listResponse.getStatusCode().is2xxSuccessful() || listResponse.getBody() == null) { | ||
| log.warn("Failed to list storage files for criminal {}", criminalId); | ||
| log.warn("Failed to list storage files for criminal {}", safeCriminalId); | ||
| return; | ||
| } | ||
|
|
||
| @@ -112,7 +115,7 @@ | ||
| JsonNode files = mapper.readTree(listResponse.getBody()); | ||
|
|
||
| if (!files.isArray() || files.isEmpty()) { | ||
| log.info("No storage files found for criminal {}", criminalId); | ||
| log.info("No storage files found for criminal {}", safeCriminalId); | ||
| return; | ||
| } | ||
|
|
||
| @@ -144,13 +147,13 @@ | ||
| deleteUrl, HttpMethod.DELETE, deleteRequest, String.class); | ||
|
|
||
| if (deleteResponse.getStatusCode().is2xxSuccessful()) { | ||
| log.info("Storage files deleted for criminal {}: {} file(s)", criminalId, prefixes.size()); | ||
| log.info("Storage files deleted for criminal {}: {} file(s)", safeCriminalId, prefixes.size()); | ||
| } else { | ||
| log.warn("Storage deletion returned status {} for criminal {}", deleteResponse.getStatusCode(), criminalId); | ||
| log.warn("Storage deletion returned status {} for criminal {}", deleteResponse.getStatusCode(), safeCriminalId); | ||
| } | ||
|
|
||
| } catch (Exception e) { | ||
| log.warn("Storage cleanup failed for criminal {}: {}", criminalId, e.getMessage()); | ||
| log.warn("Storage cleanup failed for criminal {}: {}", safeCriminalId, e.getMessage()); | ||
| } | ||
| } | ||
| } |
| JsonNode files = mapper.readTree(listResponse.getBody()); | ||
|
|
||
| if (!files.isArray() || files.isEmpty()) { | ||
| log.info("No storage files found for criminal {}", criminalId); |
Check failure
Code scanning / CodeQL
Log Injection High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 5 months ago
In general, to fix log injection issues you should sanitize or validate any user-controlled value before logging it. Common mitigations include stripping or replacing newline and carriage-return characters, optionally restricting to a safe character set, and centralizing this logic in a reusable utility method so it is consistently applied.
In this codebase, there is already a LogSanitizer utility used in CriminalController (e.g., LogSanitizer.sanitize(criminalId)), so the best fix is to apply the same sanitization in the downstream services where criminalId is logged. Specifically:
- In
CriminalService.deleteCriminal, sanitizecriminalIdbefore including it in log messages (both the warning in the catch block and the final info log). - In
SupabaseStorageService.deleteFolder, sanitizecriminalIdin all log statements (log.warnandlog.info) so that any malicious characters in the path variable cannot alter the log structure.
We will:
- Add an import for
com.crimeLink.analyzer.util.LogSanitizertoCriminalServiceandSupabaseStorageService. - Wrap
criminalIdwithLogSanitizer.sanitize(...)in all relevant log calls within the shown snippets:CriminalService: lines 197 and 202.SupabaseStorageService: lines 107, 115, 147, 149, and 153.
No behavioral change occurs other than making the logged representation of criminalId safe; the value used for business logic (DB calls, HTTP requests, etc.) remains unchanged.
| @@ -9,6 +9,7 @@ | ||
|
|
||
| import com.fasterxml.jackson.databind.JsonNode; | ||
| import com.fasterxml.jackson.databind.ObjectMapper; | ||
| import com.crimeLink.analyzer.util.LogSanitizer; | ||
|
|
||
| import java.util.ArrayList; | ||
| import java.util.List; | ||
| @@ -104,7 +105,7 @@ | ||
| listUrl, HttpMethod.POST, listRequest, String.class); | ||
|
|
||
| if (!listResponse.getStatusCode().is2xxSuccessful() || listResponse.getBody() == null) { | ||
| log.warn("Failed to list storage files for criminal {}", criminalId); | ||
| log.warn("Failed to list storage files for criminal {}", LogSanitizer.sanitize(criminalId)); | ||
| return; | ||
| } | ||
|
|
||
| @@ -112,7 +113,7 @@ | ||
| JsonNode files = mapper.readTree(listResponse.getBody()); | ||
|
|
||
| if (!files.isArray() || files.isEmpty()) { | ||
| log.info("No storage files found for criminal {}", criminalId); | ||
| log.info("No storage files found for criminal {}", LogSanitizer.sanitize(criminalId)); | ||
| return; | ||
| } | ||
|
|
||
| @@ -144,13 +145,13 @@ | ||
| deleteUrl, HttpMethod.DELETE, deleteRequest, String.class); | ||
|
|
||
| if (deleteResponse.getStatusCode().is2xxSuccessful()) { | ||
| log.info("Storage files deleted for criminal {}: {} file(s)", criminalId, prefixes.size()); | ||
| log.info("Storage files deleted for criminal {}: {} file(s)", LogSanitizer.sanitize(criminalId), prefixes.size()); | ||
| } else { | ||
| log.warn("Storage deletion returned status {} for criminal {}", deleteResponse.getStatusCode(), criminalId); | ||
| log.warn("Storage deletion returned status {} for criminal {}", deleteResponse.getStatusCode(), LogSanitizer.sanitize(criminalId)); | ||
| } | ||
|
|
||
| } catch (Exception e) { | ||
| log.warn("Storage cleanup failed for criminal {}: {}", criminalId, e.getMessage()); | ||
| log.warn("Storage cleanup failed for criminal {}: {}", LogSanitizer.sanitize(criminalId), e.getMessage()); | ||
| } | ||
| } | ||
| } |
| @@ -2,6 +2,7 @@ | ||
|
|
||
| import com.crimeLink.analyzer.entity.Criminal; | ||
| import com.crimeLink.analyzer.repository.CriminalRepository; | ||
| import com.crimeLink.analyzer.util.LogSanitizer; | ||
| import lombok.RequiredArgsConstructor; | ||
| import lombok.extern.slf4j.Slf4j; | ||
| import org.springframework.stereotype.Service; | ||
| @@ -194,12 +195,12 @@ | ||
| try { | ||
| supabaseStorageService.deleteFolder(criminalId); | ||
| } catch (Exception e) { | ||
| log.warn("Storage cleanup failed for criminal {} (non-fatal): {}", criminalId, e.getMessage()); | ||
| log.warn("Storage cleanup failed for criminal {} (non-fatal): {}", LogSanitizer.sanitize(criminalId), e.getMessage()); | ||
| } | ||
|
|
||
| // DB delete — suspect_photos cascade via ON DELETE CASCADE | ||
| criminalRepository.deleteById(criminalId); | ||
| log.info("Criminal deleted: {}", criminalId); | ||
| log.info("Criminal deleted: {}", LogSanitizer.sanitize(criminalId)); | ||
| return true; | ||
| } | ||
|
|
| deleteUrl, HttpMethod.DELETE, deleteRequest, String.class); | ||
|
|
||
| if (deleteResponse.getStatusCode().is2xxSuccessful()) { | ||
| log.info("Storage files deleted for criminal {}: {} file(s)", criminalId, prefixes.size()); |
Check failure
Code scanning / CodeQL
Log Injection High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 5 months ago
In general, to fix this kind of issue, any user-controlled data that is written to logs should be sanitized first, typically by stripping or escaping newline and other control characters and clearly delimiting user input. This ensures an attacker cannot forge extra log lines or otherwise confuse log parsers.
The best fix here, consistent with the rest of the codebase, is to apply the existing LogSanitizer.sanitize(...) helper to the criminalId before logging it in SupabaseStorageService.deleteFolder. This keeps behavior aligned with other logging in CriminalController and CriminalService, without changing any functional logic or external behavior beyond the log content. Specifically:
- Import
com.crimeLink.analyzer.util.LogSanitizerintoSupabaseStorageService. - Update the log statements in
deleteFolderthat currently logcriminalIddirectly (lines 107, 115, 147, 149, 153) to instead logLogSanitizer.sanitize(criminalId).
No new methods or complex changes are needed; we just ensure that the same sanitization used at the controller level is applied where the tainted value is actually logged inside this service.
| @@ -13,6 +13,8 @@ | ||
| import java.util.ArrayList; | ||
| import java.util.List; | ||
|
|
||
| import com.crimeLink.analyzer.util.LogSanitizer; | ||
|
|
||
| /** | ||
| * Service for uploading files to Supabase Storage. | ||
| * Uploads criminal photos to the configured bucket and returns public URLs. | ||
| @@ -104,7 +106,7 @@ | ||
| listUrl, HttpMethod.POST, listRequest, String.class); | ||
|
|
||
| if (!listResponse.getStatusCode().is2xxSuccessful() || listResponse.getBody() == null) { | ||
| log.warn("Failed to list storage files for criminal {}", criminalId); | ||
| log.warn("Failed to list storage files for criminal {}", LogSanitizer.sanitize(criminalId)); | ||
| return; | ||
| } | ||
|
|
||
| @@ -112,7 +114,7 @@ | ||
| JsonNode files = mapper.readTree(listResponse.getBody()); | ||
|
|
||
| if (!files.isArray() || files.isEmpty()) { | ||
| log.info("No storage files found for criminal {}", criminalId); | ||
| log.info("No storage files found for criminal {}", LogSanitizer.sanitize(criminalId)); | ||
| return; | ||
| } | ||
|
|
||
| @@ -144,13 +146,13 @@ | ||
| deleteUrl, HttpMethod.DELETE, deleteRequest, String.class); | ||
|
|
||
| if (deleteResponse.getStatusCode().is2xxSuccessful()) { | ||
| log.info("Storage files deleted for criminal {}: {} file(s)", criminalId, prefixes.size()); | ||
| log.info("Storage files deleted for criminal {}: {} file(s)", LogSanitizer.sanitize(criminalId), prefixes.size()); | ||
| } else { | ||
| log.warn("Storage deletion returned status {} for criminal {}", deleteResponse.getStatusCode(), criminalId); | ||
| log.warn("Storage deletion returned status {} for criminal {}", deleteResponse.getStatusCode(), LogSanitizer.sanitize(criminalId)); | ||
| } | ||
|
|
||
| } catch (Exception e) { | ||
| log.warn("Storage cleanup failed for criminal {}: {}", criminalId, e.getMessage()); | ||
| log.warn("Storage cleanup failed for criminal {}: {}", LogSanitizer.sanitize(criminalId), e.getMessage()); | ||
| } | ||
| } | ||
| } |
| if (deleteResponse.getStatusCode().is2xxSuccessful()) { | ||
| log.info("Storage files deleted for criminal {}: {} file(s)", criminalId, prefixes.size()); | ||
| } else { | ||
| log.warn("Storage deletion returned status {} for criminal {}", deleteResponse.getStatusCode(), criminalId); |
Check failure
Code scanning / CodeQL
Log Injection High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 5 months ago
In general, any user-controlled value written to logs should be sanitized or strictly validated before logging. In this codebase, the established pattern is to use LogSanitizer.sanitize(...) wherever request parameters or path variables are logged. We should apply the same pattern to SupabaseStorageService.deleteFolder (and ideally to other service logs using criminalId), ensuring the logged value cannot contain line breaks or other problematic characters, while still passing the raw value to business logic (e.g., Supabase API and repository calls) so functionality is unchanged.
The best targeted fix is:
- Import
com.crimeLink.analyzer.util.LogSanitizerintoSupabaseStorageService. - In
deleteFolder, compute a localString safeCriminalId = LogSanitizer.sanitize(criminalId);. - Use
safeCriminalIdin all log statements instead ofcriminalId. - Optionally, mirror this in
CriminalService.deleteCriminalwherecriminalIdis also logged unsanitized (this is on the same taint path and within the provided snippet).
These changes:
- Affect only logging, not the values sent to Supabase or the database.
- Reuse an existing utility, so no new dependencies are required.
- Maintain current behavior aside from making logs robust against injection.
| @@ -13,6 +13,8 @@ | ||
| import java.util.ArrayList; | ||
| import java.util.List; | ||
|
|
||
| import com.crimeLink.analyzer.util.LogSanitizer; | ||
|
|
||
| /** | ||
| * Service for uploading files to Supabase Storage. | ||
| * Uploads criminal photos to the configured bucket and returns public URLs. | ||
| @@ -88,6 +90,7 @@ | ||
| * @param criminalId The criminal's ID (used as the folder prefix) | ||
| */ | ||
| public void deleteFolder(String criminalId) { | ||
| String safeCriminalId = LogSanitizer.sanitize(criminalId); | ||
| try { | ||
| // 1) List objects in the folder | ||
| String listUrl = supabaseUrl + "/storage/v1/object/list/" + bucket; | ||
| @@ -104,7 +107,7 @@ | ||
| listUrl, HttpMethod.POST, listRequest, String.class); | ||
|
|
||
| if (!listResponse.getStatusCode().is2xxSuccessful() || listResponse.getBody() == null) { | ||
| log.warn("Failed to list storage files for criminal {}", criminalId); | ||
| log.warn("Failed to list storage files for criminal {}", safeCriminalId); | ||
| return; | ||
| } | ||
|
|
||
| @@ -112,7 +115,7 @@ | ||
| JsonNode files = mapper.readTree(listResponse.getBody()); | ||
|
|
||
| if (!files.isArray() || files.isEmpty()) { | ||
| log.info("No storage files found for criminal {}", criminalId); | ||
| log.info("No storage files found for criminal {}", safeCriminalId); | ||
| return; | ||
| } | ||
|
|
||
| @@ -144,13 +147,13 @@ | ||
| deleteUrl, HttpMethod.DELETE, deleteRequest, String.class); | ||
|
|
||
| if (deleteResponse.getStatusCode().is2xxSuccessful()) { | ||
| log.info("Storage files deleted for criminal {}: {} file(s)", criminalId, prefixes.size()); | ||
| log.info("Storage files deleted for criminal {}: {} file(s)", safeCriminalId, prefixes.size()); | ||
| } else { | ||
| log.warn("Storage deletion returned status {} for criminal {}", deleteResponse.getStatusCode(), criminalId); | ||
| log.warn("Storage deletion returned status {} for criminal {}", deleteResponse.getStatusCode(), safeCriminalId); | ||
| } | ||
|
|
||
| } catch (Exception e) { | ||
| log.warn("Storage cleanup failed for criminal {}: {}", criminalId, e.getMessage()); | ||
| log.warn("Storage cleanup failed for criminal {}: {}", safeCriminalId, e.getMessage()); | ||
| } | ||
| } | ||
| } |
| @@ -10,6 +10,8 @@ | ||
| import java.time.LocalDate; | ||
| import java.util.*; | ||
|
|
||
| import com.crimeLink.analyzer.util.LogSanitizer; | ||
|
|
||
| /** | ||
| * Service for direct criminal record CRUD operations against the database. | ||
| * Handles profile data management and coordinates with ML service for embeddings. | ||
| @@ -174,7 +176,8 @@ | ||
| } | ||
|
|
||
| Criminal saved = criminalRepository.save(c); | ||
| log.info("Criminal updated: {}", criminalId); | ||
| String safeCriminalIdForUpdate = LogSanitizer.sanitize(criminalId); | ||
| log.info("Criminal updated: {}", safeCriminalIdForUpdate); | ||
|
|
||
| return Optional.of(toDetailMap(saved)); | ||
| } | ||
| @@ -190,16 +193,18 @@ | ||
| return false; | ||
| } | ||
|
|
||
| String safeCriminalId = LogSanitizer.sanitize(criminalId); | ||
|
|
||
| // Best-effort: delete photos from Supabase Storage | ||
| try { | ||
| supabaseStorageService.deleteFolder(criminalId); | ||
| } catch (Exception e) { | ||
| log.warn("Storage cleanup failed for criminal {} (non-fatal): {}", criminalId, e.getMessage()); | ||
| log.warn("Storage cleanup failed for criminal {} (non-fatal): {}", safeCriminalId, e.getMessage()); | ||
| } | ||
|
|
||
| // DB delete — suspect_photos cascade via ON DELETE CASCADE | ||
| criminalRepository.deleteById(criminalId); | ||
| log.info("Criminal deleted: {}", criminalId); | ||
| log.info("Criminal deleted: {}", safeCriminalId); | ||
| return true; | ||
| } | ||
|
|
| } | ||
|
|
||
| } catch (Exception e) { | ||
| log.warn("Storage cleanup failed for criminal {}: {}", criminalId, e.getMessage()); |
Check failure
Code scanning / CodeQL
Log Injection High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 5 months ago
In general, to fix log injection, any user-controlled or otherwise untrusted data should be sanitized (e.g., stripping newlines and control characters) before being written to logs. This can be done either inline at each log call or via a reusable helper/utility.
In this codebase, the CriminalController already uses LogSanitizer.sanitize(criminalId) when logging criminalId. The best fix that preserves existing functionality is to apply the same sanitization in the service layer where criminalId is logged: in CriminalService and SupabaseStorageService. Specifically:
- Import
com.crimeLink.analyzer.util.LogSanitizerintoCriminalServiceandSupabaseStorageService. - Wrap every occurrence of
criminalId(and similar user-controlled values) passed tolog.*calls in these classes withLogSanitizer.sanitize(...). - Do not change the behavior of non-logging code (e.g., values used for repository lookups or Supabase requests).
Concretely:
- In
CriminalService.deleteCriminal, change:log.warn("Storage cleanup failed for criminal {} (non-fatal): {}", criminalId, e.getMessage());log.info("Criminal deleted: {}", criminalId);
to useLogSanitizer.sanitize(criminalId).
- In
SupabaseStorageService.uploadPhoto, change:log.error("Failed to upload photo for criminal {}: {}", criminalId, e.getMessage());
- In
SupabaseStorageService.deleteFolder, change all log calls that includecriminalId:log.warn("Failed to list storage files for criminal {}", criminalId);log.info("No storage files found for criminal {}", criminalId);log.info("Storage files deleted for criminal {}: {} file(s)", criminalId, prefixes.size());log.warn("Storage deletion returned status {} for criminal {}", deleteResponse.getStatusCode(), criminalId);log.warn("Storage cleanup failed for criminal {}: {}", criminalId, e.getMessage());
to useLogSanitizer.sanitize(criminalId).
This requires only adding the import and wrapping arguments, without modifying method signatures or business logic.
| @@ -10,6 +10,7 @@ | ||
| import com.fasterxml.jackson.databind.JsonNode; | ||
| import com.fasterxml.jackson.databind.ObjectMapper; | ||
|
|
||
| import com.crimeLink.analyzer.util.LogSanitizer; | ||
| import java.util.ArrayList; | ||
| import java.util.List; | ||
|
|
||
| @@ -76,7 +77,7 @@ | ||
| throw new RuntimeException("Failed to upload photo to storage. Status: " + response.getStatusCode()); | ||
| } | ||
| } catch (Exception e) { | ||
| log.error("Failed to upload photo for criminal {}: {}", criminalId, e.getMessage()); | ||
| log.error("Failed to upload photo for criminal {}: {}", LogSanitizer.sanitize(criminalId), e.getMessage()); | ||
| throw new RuntimeException("Photo upload failed: " + e.getMessage(), e); | ||
| } | ||
| } | ||
| @@ -104,7 +105,7 @@ | ||
| listUrl, HttpMethod.POST, listRequest, String.class); | ||
|
|
||
| if (!listResponse.getStatusCode().is2xxSuccessful() || listResponse.getBody() == null) { | ||
| log.warn("Failed to list storage files for criminal {}", criminalId); | ||
| log.warn("Failed to list storage files for criminal {}", LogSanitizer.sanitize(criminalId)); | ||
| return; | ||
| } | ||
|
|
||
| @@ -112,7 +113,7 @@ | ||
| JsonNode files = mapper.readTree(listResponse.getBody()); | ||
|
|
||
| if (!files.isArray() || files.isEmpty()) { | ||
| log.info("No storage files found for criminal {}", criminalId); | ||
| log.info("No storage files found for criminal {}", LogSanitizer.sanitize(criminalId)); | ||
| return; | ||
| } | ||
|
|
||
| @@ -144,13 +145,13 @@ | ||
| deleteUrl, HttpMethod.DELETE, deleteRequest, String.class); | ||
|
|
||
| if (deleteResponse.getStatusCode().is2xxSuccessful()) { | ||
| log.info("Storage files deleted for criminal {}: {} file(s)", criminalId, prefixes.size()); | ||
| log.info("Storage files deleted for criminal {}: {} file(s)", LogSanitizer.sanitize(criminalId), prefixes.size()); | ||
| } else { | ||
| log.warn("Storage deletion returned status {} for criminal {}", deleteResponse.getStatusCode(), criminalId); | ||
| log.warn("Storage deletion returned status {} for criminal {}", deleteResponse.getStatusCode(), LogSanitizer.sanitize(criminalId)); | ||
| } | ||
|
|
||
| } catch (Exception e) { | ||
| log.warn("Storage cleanup failed for criminal {}: {}", criminalId, e.getMessage()); | ||
| log.warn("Storage cleanup failed for criminal {}: {}", LogSanitizer.sanitize(criminalId), e.getMessage()); | ||
| } | ||
| } | ||
| } |
| @@ -2,6 +2,7 @@ | ||
|
|
||
| import com.crimeLink.analyzer.entity.Criminal; | ||
| import com.crimeLink.analyzer.repository.CriminalRepository; | ||
| import com.crimeLink.analyzer.util.LogSanitizer; | ||
| import lombok.RequiredArgsConstructor; | ||
| import lombok.extern.slf4j.Slf4j; | ||
| import org.springframework.stereotype.Service; | ||
| @@ -194,12 +195,12 @@ | ||
| try { | ||
| supabaseStorageService.deleteFolder(criminalId); | ||
| } catch (Exception e) { | ||
| log.warn("Storage cleanup failed for criminal {} (non-fatal): {}", criminalId, e.getMessage()); | ||
| log.warn("Storage cleanup failed for criminal {} (non-fatal): {}", LogSanitizer.sanitize(criminalId), e.getMessage()); | ||
| } | ||
|
|
||
| // DB delete — suspect_photos cascade via ON DELETE CASCADE | ||
| criminalRepository.deleteById(criminalId); | ||
| log.info("Criminal deleted: {}", criminalId); | ||
| log.info("Criminal deleted: {}", LogSanitizer.sanitize(criminalId)); | ||
| return true; | ||
| } | ||
|
|
There was a problem hiding this comment.
Pull request overview
This PR adds backend support for criminal record CRUD operations, integrates Supabase Storage for photo uploads, and extends the facial-recognition ML integration to handle additional criminal profile fields and embedding generation.
Changes:
- Add Supabase Storage configuration and a service to upload/delete criminal photos.
- Introduce direct DB-backed criminal CRUD service/controller with optional photo upload + embedding generation.
- Expand ML registration payload fields and add an ML embedding-generation call; update security rules for new/updated endpoints.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/resources/application.properties | Adds Supabase Storage configuration properties. |
| src/main/java/com/crimeLink/analyzer/service/SupabaseStorageService.java | New service to upload photos to Supabase and attempt folder cleanup. |
| src/main/java/com/crimeLink/analyzer/service/FacialRecognitionService.java | Extends registration payload and adds generateEmbedding() ML call. |
| src/main/java/com/crimeLink/analyzer/service/CriminalService.java | New DB CRUD service coordinating photo storage + embedding generation. |
| src/main/java/com/crimeLink/analyzer/repository/CriminalRepository.java | Adds query to fetch IDs with non-null face embeddings. |
| src/main/java/com/crimeLink/analyzer/entity/Criminal.java | Adds fields for risk level, history, photo URL, DOB, gender, alias. |
| src/main/java/com/crimeLink/analyzer/controller/FacialRecognitionController.java | Extends request parameters forwarded to the ML register endpoint. |
| src/main/java/com/crimeLink/analyzer/controller/CriminalController.java | New REST controller exposing /api/criminals CRUD endpoints. |
| src/main/java/com/crimeLink/analyzer/config/SecurityConfig.java | Adjusts role access rules for facial endpoints and adds rules for /api/criminals. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| Map<String, Object> result = new LinkedHashMap<>(); | ||
| result.put("criminal_id", saved.getId()); | ||
| result.put("name", saved.getName()); | ||
| result.put("nic", saved.getNic()); |
There was a problem hiding this comment.
createCriminal() returns the identifier under criminal_id, but the list/detail mappings use id. This makes the /api/criminals API inconsistent for clients. Consider standardizing the key name across all responses (e.g., always id or always criminal_id).
|
|
||
| // Criminal CRUD (direct DB, no Python) | ||
| .requestMatchers("/api/criminals/**").hasAnyRole("Investigator", "OIC") | ||
| .requestMatchers("/api/criminals").hasAnyRole("Investigator", "OIC") |
There was a problem hiding this comment.
/api/criminals/** already matches /api/criminals, so the additional matcher for /api/criminals is redundant and never reached. Removing the duplicate reduces confusion about which rule applies.
| .requestMatchers("/api/criminals").hasAnyRole("Investigator", "OIC") |
| @Value("${supabase.bucket}") | ||
| private String bucket; | ||
|
|
||
| private final RestTemplate restTemplate = new RestTemplate(); |
There was a problem hiding this comment.
SupabaseStorageService instantiates a new RestTemplate directly, which bypasses the configured RestTemplate bean (timeouts, shared config) used elsewhere (see RestTemplateConfig). Inject RestTemplate via constructor/@requiredargsconstructor instead of new RestTemplate() so calls don’t hang indefinitely and configuration stays consistent.
| // 3) Bulk delete via POST /storage/v1/object/remove | ||
| String deleteUrl = supabaseUrl + "/storage/v1/object/" + bucket; | ||
|
|
||
| HttpHeaders deleteHeaders = new HttpHeaders(); | ||
| deleteHeaders.set("Authorization", "Bearer " + supabaseServiceKey); | ||
| deleteHeaders.set("apikey", supabaseServiceKey); | ||
| deleteHeaders.setContentType(MediaType.APPLICATION_JSON); | ||
|
|
||
| String deleteBody = mapper.writeValueAsString(new java.util.LinkedHashMap<String, Object>() {{ | ||
| put("prefixes", prefixes); | ||
| }}); | ||
| HttpEntity<String> deleteRequest = new HttpEntity<>(deleteBody, deleteHeaders); | ||
|
|
||
| ResponseEntity<String> deleteResponse = restTemplate.exchange( | ||
| deleteUrl, HttpMethod.DELETE, deleteRequest, String.class); | ||
|
|
There was a problem hiding this comment.
The bulk-delete call doesn’t match the stated endpoint: the comment says "POST /storage/v1/object/remove" but the code sends an HTTP DELETE to /storage/v1/object/{bucket} with a JSON body containing prefixes. Supabase Storage’s bulk remove API is a POST to the /object/remove endpoint (and typically expects a list of paths), while DELETE is for single-object deletion. As written, this is likely to 404/405 and leave files undeleted; align the URL/method/payload with the bulk remove endpoint.
| String listBody = "{\"prefix\":\"" + criminalId + "/\",\"limit\":100}"; | ||
| HttpEntity<String> listRequest = new HttpEntity<>(listBody, listHeaders); |
There was a problem hiding this comment.
listBody is built via string concatenation with criminalId, which can break JSON encoding if the id ever contains quotes/backslashes and makes the request harder to maintain. Build the JSON payload via ObjectMapper/Map so values are properly escaped, and consider pagination/looping since limit is fixed to 100 (folders with >100 objects won’t be fully deleted).
| String deleteBody = mapper.writeValueAsString(new java.util.LinkedHashMap<String, Object>() {{ | ||
| put("prefixes", prefixes); | ||
| }}); |
There was a problem hiding this comment.
deleteBody is constructed using double-brace initialization (new LinkedHashMap<>() {{ ... }}), which creates an extra anonymous class and can retain references unexpectedly. Prefer creating a normal Map and putting values explicitly before serializing.
| String deleteBody = mapper.writeValueAsString(new java.util.LinkedHashMap<String, Object>() {{ | |
| put("prefixes", prefixes); | |
| }}); | |
| java.util.Map<String, Object> deleteBodyMap = new java.util.LinkedHashMap<>(); | |
| deleteBodyMap.put("prefixes", prefixes); | |
| String deleteBody = mapper.writeValueAsString(deleteBodyMap); |
changed sidebars